Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Regex to Reduce Matching Time #4160

Merged
merged 12 commits into from
Nov 27, 2024

Conversation

thomhurst
Copy link
Contributor

Partially fixes #4159

@nohwnd
Copy link
Member

nohwnd commented Nov 27, 2024

The new regex will not match the first line (or the other lines shown in red)

current:
image

new:
image

@Youssef1313
Copy link
Member

@nohwnd That means we need better test coverage :)

@nohwnd
Copy link
Member

nohwnd commented Nov 27, 2024

ofc

@nohwnd
Copy link
Member

nohwnd commented Nov 27, 2024

I thought it would be caught by OutputFormattingIsCorrect test, but that runs only on code we have symbols for, so the line is there. So probably best to add unit test for this, once we figure out what the problem is, if that is just the system being really slow, or some input throwing the regex into a really bad state.

@thomhurst
Copy link
Contributor Author

Ah good shout! I've just added a test for the regex. What do you think?

@nohwnd
Copy link
Member

nohwnd commented Nov 27, 2024

The test looks good.

@thomhurst
Copy link
Contributor Author

A very marginal improvement on the regex performance, but an improvement none-the-less:

https://github.com/thomhurst/MSTP-Stacktrace-Regex-Benchmark/actions/runs/12053547703/job/33609479485#step:4:660

@thomhurst thomhurst changed the title Simplify Regex to Reduce Matching Time Improve Regex to Reduce Matching Time Nov 27, 2024
@Evangelink
Copy link
Member

@thomhurst I haven't followed in details teh discussion. Is the PR already handling the timeout case that you reported in the issue?

@microsoft-github-policy-service microsoft-github-policy-service bot added Area: Terminal reporter Area: Testing Platform Belongs to the Microsoft.Testing.Platform core library labels Nov 27, 2024
@thomhurst
Copy link
Contributor Author

thomhurst commented Nov 27, 2024

It doesn't handle the timeout, but it improves performance and adds a specific AOT stacktrace regex:

https://github.com/thomhurst/MSTP-Stacktrace-Regex-Benchmark/actions/runs/12055817470/job/33618423983#step:4:2023

@Evangelink
Copy link
Member

Ok let me merge this one and make sure the timeout part remains open.

@Evangelink Evangelink merged commit 70546c0 into microsoft:main Nov 27, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Terminal reporter Area: Testing Platform Belongs to the Microsoft.Testing.Platform core library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Text.RegularExpressions.RegexMatchTimeoutException
5 participants